-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reformat exports #2058
Reformat exports #2058
Conversation
40bf5df
to
46be171
Compare
46be171
to
e3de313
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions
@@ -207,4 +207,4 @@ class FilterCreator extends React.PureComponent<CombinedProps, State> { | |||
}; | |||
} | |||
|
|||
export default withAppProvider<Props>()(FilterCreator); | |||
export default withAppProvider<FilterCreatorProps>()(FilterCreator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
|
||
export {Props} from './FilterCreator'; | ||
export default FilterCreator; | ||
export {default as FilterCreator, FilterCreatorProps} from './FilterCreator'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -133,4 +133,4 @@ function buildOperatorOptions(operatorText?: string | Operator[]) { | |||
}); | |||
} | |||
|
|||
export default withAppProvider<Props>()(FilterValueSelector); | |||
export default withAppProvider<FilterValueSelectorProps>()(FilterValueSelector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
export {Props} from './FilterValueSelector'; | ||
export default FilterValueSelector; | ||
export { | ||
default as FilterValueSelector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
|
||
describe('<UserMenu />', () => { | ||
const userMenuProps: Props = { | ||
const userMenuProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need the typehint here, then we can avoid importing UserMenuProps entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, does TS infer the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it'll complain if we miss a prop that needs to be passed in
My comments make more sense in split diff view |
@@ -1,8 +1,8 @@ | |||
export {Navigation, NavigationProps} from './Navigation'; | |||
|
|||
export { | |||
ItemProps, | |||
ItemProps as NavigationItemProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for this PR, but let's revisit this, it might make sense to rename the entire component
SubNavigationItem, | ||
isNavigationItemActive, | ||
MessageProps, | ||
MessageProps as NavigationMessageProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Tidy up a few bits of spacing along the way so consistently have a single line between export declarations
Add several exceptions for cases where we still use a default export because we have to wrap things in withAppProvider. Their number shall shrink over time as we migrate to use hooks for these components
Some stuff in web touches this private type. They're doing a naughty thing but lets keep the status quo for now and we can chide them later
787d4d7
to
42ed566
Compare
WHY are these changes introduced?
Most of an implementation for #1959. This is a big change so I'm going to split it into a few commits:
WHAT is this pull request doing?
Updates all components (and sub-components and sub-sub-components) so that:
Button
instead of a default exportButtonProps
instead ofProps
This means that in component (/ subcomponent) indexes we don't need to do any renaming of exports when reexporting them, things keep the same name throughout the hierarchy.
There's still a few default exports used because of how we export things in withAppProvider and I think it is better to leave them be for the moment, The fix for them can be added in a separate PR where the changes won't get lost in all the other noise.
You can use the following script to search for items that have default or
Props
exports